Fix detected tokens added to wrong network #22814
Conversation
… brian/token-detection-network-switch
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #22814 +/- ##
===========================================
- Coverage 68.48% 68.46% -0.02%
===========================================
Files 1088 1088
Lines 42886 42826 -60
Branches 11418 11395 -23
===========================================
- Hits 29370 29319 -51
+ Misses 13516 13507 -9 ☔ View full report in Codecov by Sentry. |
darkwing
added
the
needs-assets-ux-review
|
A related thing I want to fix separately. We run detection 3 times when switching chains:
Only needs to run once. |
Builds ready [5e6b428]
Page Load Metrics (816 ± 20 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| tokensController?.subscribe( | ||
| ({ tokens = [], ignoredTokens = [], detectedTokens = [] }) => { | ||
| this.tokenAddresses = tokens.map((token) => { | ||
| return token.address; | ||
| }); | ||
| this.hiddenTokens = ignoredTokens; | ||
| this.detectedTokens = detectedTokens; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
tokensController?.subscribe was typically the last event to fire during a chain switch. Therefore tokenAddresses, hiddenTokens, and detectedTokens were still from the old chain during detection, even when (most) everything else is pointing to the new chain. Since tokensController has state per-chain, we can just grab the tokens for the correct chain instead of waiting to be updated via subscription.
| const chainIdAgainstWhichToDetect = | ||
| chainId ?? this.getChainIdFromNetworkStore(); |
There was a problem hiding this comment.
During a network switch, this.chainId is from the old chain until NetworkController:stateChange fires and updates it. But TokenListController:stateChange can fire first, leaving it stale at this point. This change gets the updated chain id from the network controller, as happens elsewhere in this file.
| this.network.findNetworkClientIdByChainId( | ||
| chainIdAgainstWhichToDetect, | ||
| ), |
There was a problem hiding this comment.
This feels like maybe the sketchiest one. We weren't passing an explicit NetworkClientId here, which means AssetsContractController falls back to the chain in its state. But that controller's onNetworkDidChange has not always fired yet, so its still on the old chain, and we detect the new token addresses on the old chain.
This was the only way I found to go from chain id -> NetworkClientId since that's what getBalancesInSingleCall accepts.
There was a problem hiding this comment.
Hey Brian, yeah so this is the best option available for right now. You've correctly identified an issue with this flow. We have implemented an alternative polling solution that we will be leveraging shortly but you've highlighted one piece that we're missing. @shanejonas we will need to pass along the networkClientId in the _executePoll method.
There was a problem hiding this comment.
created a ticket to do that cleanup: https://app.zenhub.com/workspaces/devex-63bee08a4e3b9d001108416e/issues/gh/metamask/metamask-planning/2097
There was a problem hiding this comment.
LGTM! Thanks for the fix! And totally agree with your comment that we should, in a subsequent PR, make sure the restartTokenDetection only fires once after network changes!
metamaskbot
added
the
release-11.12.0
Description
Fixes a race condition during token detection after switching networks.
During the network switch, some controllers state are still on the old chain id, and others are on the new chain id. This can cause different issues depending which controllers win the race. The worst case is that detected tokens are added to the wrong network (see related issues):
In that ^ screenshot there are 2 mainnet tokens that we have a balance of, but they incorrectly appear under linea.
There's a fix for each of the relevant controllers (
DetectTokensController,TokensController,AssetsContractController) ensuring we use the chain ID being switched to.Related issues
Auto token detection list collision with other networks #22512
Autodetect tokens display Mainnet tokens on another network #7587
Manual testing steps
It takes a few minutes of switching networks back and forth to reproduce the bug. But basically keep doing that and we should not see tokens hop from 1 network to the other.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist